Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for npm dependencies + improved output #45

Merged
merged 10 commits into from
Jan 29, 2016
Merged

Conversation

kategengler
Copy link
Member

  • Uses the config proposed in NPM scenario support #26, with an npm and bower key under each scenario, then the dependencies and devDependencies (and resolutions, for bower) under that. The old configuration format is still supported, but only for bower dependencies.
  • Supports npm dependencies, at least some way: Backs up node_modules and package.json before any commands are run, modifies package.json based on the scenario, runs npm install and npm prune, runs the command, and to cleanup, restores the package.json and node_modules folder and runs npm install and npm prune.
  • Unifies the code path for ember try and ember try:testall
  • New output for the results of running a scenario that shows the dependencies that were changed, what was specified in the config and the actual version that was used.
    Image of output
  • Many more tests
  • Upgrade ember-cli

Limitation: Currently packages can be removed between scenarios (thanks to npm prune) but packages in package.json cannot be removed. This needs further thought and perhaps our own configuration syntax. This is the same as the current support for bower.

Further things to do:

  • Need checks against trying to vary ember-try between scenarios.
  • Use semver comparison to determine status of dependencies in output table (right now strict comparison)
  • Maybe warn if trying to vary ember-cli between scenarios.
  • Find out if this works on Windows.

Important Note: When specifying packages for npm in scenarios, remember to put them in the same "bucket" as in package.json -- devDependencies go under devDependencies, and so forth.

- Extract 'runCommand' common pattern
- Various cleanup and changes that were needed for testability
[Katie & Michelle]
test
- Remove tasks/test and just use tasks/try-each, for a single scenario
- Inject dependencyManagerAdapter & speed up some tests
- Appease JSCS
- Add note about some tests using a dependency manager for real
- Use ember try:testall for "client" test for a smoke test
- Remove dead code
- Fix test for findBowerPath
- Support multiple package managers
- Tests for npm adapter that I'm *sure* won't be flaky
@kategengler kategengler mentioned this pull request Jan 26, 2016
4 tasks
@pangratz
Copy link

This looks awesome! Having npm support for ember-try rocks!

This will greatly help in https://github.com/switchfly/ember-test-helpers, where the ember-data-2.3 scenario needs to include ember-data as an addon via npm.

I created emberjs/ember-test-helpers#145 to use the latest ember-try code of this PR, but it seems that there is a problem when beta and canary versions for ember are resolved https://travis-ci.org/switchfly/ember-test-helpers/builds/105231965#L2376-L2379:

bower resolved      git://github.com/components/ember.git#b27df6ad05
bower resolution    Unsuitable resolution declared for ember: components/ember#release
bower ECONFLICT     Unable to find suitable version for ember
Error!
1
undefined

I just wanted to let you know of this and give you feedback on this. I don't know how finished this PR is, so please ignore if the current state shouldn't be tried out yet...

Anyway, thanks again for this!

@kategengler
Copy link
Member Author

@pangratz Thanks for trying it out. Looks like I messed something up where the resolutions for bower are no longer set to what they are in the config, so that's a good catch. I'll try to update this tonight.

@pangratz
Copy link

Awesome, thank you kindly!

- Add test at that level
- Change integration tests to use config including all types of options
@kategengler
Copy link
Member Author

@pangratz Should be all set. Note that to use npm deps, you need to use the new config syntax, see the upgraded README

@pangratz
Copy link

Thanks for the heads up. I have good news: emberjs/ember-test-helpers#145 is all green now! Everything works as expected.

🎉 🎈 👍

@kategengler
Copy link
Member Author

Great!

kategengler added a commit that referenced this pull request Jan 29, 2016
Support for npm dependencies + improved output
@kategengler kategengler merged commit 987173f into master Jan 29, 2016
@kategengler kategengler deleted the npm-support branch January 29, 2016 19:04
@rwjblue
Copy link
Member

rwjblue commented Jan 29, 2016

❤️

luxzeitlos pushed a commit to luxzeitlos/ember-try that referenced this pull request Sep 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants